Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add --lifecycleRule option with improved validation #182

Closed
wants to merge 2 commits into from

Conversation

mjurbanski-reef
Copy link

@mjurbanski-reef mjurbanski-reef commented Jul 5, 2023

TODO

CHANGELOG.md Outdated Show resolved Hide resolved
b2/_cli/obj_loads.py Show resolved Hide resolved
b2/console_tool.py Show resolved Hide resolved
@@ -109,6 +109,7 @@ def read_requirements(extra=None):
# for example:
# $ pip install -e .[dev,test]
extras_require={
'full': read_requirements('full'),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

full means it's something missing from the original, while this is here only to provide a higher order validation of some inputs. How about validation or typechecks, or even just checks?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, but it is missing, and we only allow installing without since we acknowledge some people may run into issues with the additional dependencies.

Then there is also this - Assume we add pycurl support, then if you are a user which want the best experience possible, and you don't care if there are binary dependencies or whatnot (and why would you as the user?), do you really want to do pip install b2[pycurl,validation]` ?

It seems like a common practice to group them (https://github.com/encode/uvicorn/blob/master/pyproject.toml#L36-L44 , https://github.com/pandas-dev/pandas/blob/main/pyproject.toml#L81 ) .
We could do add extra group for just validation boost, for these users that do really care about nitpicking their dependencies, but I'm not sure how big of group will that be.

Copy link

@kkalinowski-reef kkalinowski-reef Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, but in these cases all means that literally all dependencies are added, and each of them have a hefty list of optionals that can be added as well. So it should also include doc and license, and curl and everything else in this full, shouldn't it? Currently it's just adding pydantic, which is used only for validation, isn't it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against full, or all. I'm just saying that this full is not really full.

test/unit/test_console_tool.py Show resolved Hide resolved
@mjurbanski-reef mjurbanski-reef force-pushed the typed_lifecycle_rules branch 2 times, most recently from 6336bd3 to cb6efbf Compare August 10, 2023 15:51
@mjurbanski-reef
Copy link
Author

merged upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants